Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster version of from_shape_fn and speed up ndarray-rand #728

Merged
merged 4 commits into from
Oct 13, 2019

Conversation

bluss
Copy link
Member

@bluss bluss commented Sep 24, 2019

Add new constructor from_shape_simple_fn

This is like from_shape_fn, but avoids the overhead of iterating the
indices for the elements - instead elements are just visited in memory
order.

Then, use from_shape_simple_fn in ndarray-rand, since
it doesn't care for indices, it generates items randomly.

Quick benchmarks (the existing ndarray-rand benches) show improvements, especially in the simpler uniform distribution:

 name     before ns/iter  after ns/iter  diff ns/iter   diff % 
 norm_f32     247,189     233,467          -13,722   -5.55% 
 norm_f64     239,662     223,620          -16,042   -6.69% 
 uniform_f32  95,626      59,371           -36,255  -37.91%

@bluss
Copy link
Member Author

bluss commented Sep 24, 2019

I idly wonder if we can just turn this around and implement internal iteration - fold - for the indices iterators, and use that instead. It's still unlikely to generalize as well as this.

@bluss
Copy link
Member Author

bluss commented Sep 24, 2019

One can also see that the ndarray-rand crate has no substance beyond this line:

Array::from_shape_fn(shape, |_| dist.sample(rng))

The whole thing is a wrapper for that + a method with a default for the rng. 🙂

@bluss
Copy link
Member Author

bluss commented Sep 27, 2019

I don't really like the name from_shape_fn_memory_order but can't think of a better one. from_shape_fn_linear ? It's likely to cause confusion. from_shape_fn_linear_index?

@jturner314
Copy link
Member

I think this is a good idea, and the implementation looks good. I agree, though, that from_shape_fn_memory_order needs a better name, since the existence of a method named from_shape_fn_memory_order suggests that from_shape_fn doesn't iterate in memory order. Maybe from_shape_fn_offset to indicate the the closure's parameter is the memory offset from the first element? Alternatively, we could rename from_shape_fn to from_shape_indexed_fn and add a new from_shape_fn method that doesn't pass anything to the closure.

@bluss
Copy link
Member Author

bluss commented Sep 28, 2019

Now it has the name from_shape_simple_fn. It more or less sacrifices the index as parameter - now the closure is called with no parameter, so that it has an easier to understand name or explanation. We can do it either way, IMO.

This is like from_shape_fn, but avoids the overhead of iterating the
indices for the elements - instead elements are just visited in memory
order.
This simplifies random/random_using methods by skipping the index
iteration in the constructor, which is unused anyway and only used for
figuring out how many elments to make.
And let the closure take no argument.

The rationale is that if a counter is needed, it can easily be added by
the caller. It's a duplicate counter then, but with luck it could be
conflated with the other by the compiler?

Use it in Array::default, the other usages are not significant.
@bluss bluss merged commit 792e17c into master Oct 13, 2019
@bluss bluss deleted the rand-from-fn-opt branch October 13, 2019 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants